Skip to content

Conversation

@didoo
Copy link
Contributor

@didoo didoo commented May 9, 2025

📌 Summary

While working on hashicorp/vault#30555 I discovered a strange "bug" in the Hds::Form::Label: essentially in the case of a radio group (an Hds::Form::Field) where the possible values are booleans true/false and the values are used also as ID for the elements, the for HTML attribute behaves differently than the id HTML attribute:

  • for={{true}} in the template → renders as for attribute, without value
  • for={{false}} in the template → it does not render the for attribute
  • id={{true}} in the template → renders as id="true" attribute
  • id={{false}} in the template → renders as id="false" attribute

This means that, while for the false case the association between the id of the input and the for of the label is maintained, because the ID is generated as guidFor() by the getElementId() function, in the true case this association is broken, because the getElementId() function sees that an ID is provided (element.args.id evaluates to true), and so it passes this argument to the id of the input, which is converted to a string, and passed to the for of the label, which is not converted to a string but simply omitted.

screenshot_4956

To fix this discrepancy, I have just wrapped in double quotes the for assignment in the handlebars template of the Form::Label component.

🛠️ Detailed description

In this PR I have:

  • forced for attribute in Hds::Form::Label to be a string
  • added special use case for Hds::Form::Radio::Group with boolean values to showcase
  • added integration test for
    • Hds::Form::Label for use case when @controlId is a boolean
    • Hds::Form::Radio::Group for use case when RadioField @id is a boolean

👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

@didoo didoo requested a review from a team as a code owner May 9, 2025 15:39
@vercel
Copy link

vercel bot commented May 9, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview May 9, 2025 4:35pm
hds-website ✅ Ready (Inspect) Visit Preview May 9, 2025 4:35pm

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes an issue where the Hds::Form::Label's "for" attribute is not properly rendered when the @controlid argument is a boolean, ensuring consistent behavior for both true and false values.

  • Forced the "for" attribute to be converted to a string
  • Added an integration test to verify proper rendering with boolean values
  • Updated the changeset patch note to reflect the changes

Reviewed Changes

Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.

File Description
showcase/tests/integration/components/hds/form/label/index-test.js Added integration tests to verify that the "for" attribute is rendered as a string even when @controlid is a boolean.
.changeset/nine-fans-cough.md Updated changeset with patch note detailing the forced string conversion of the "for" attribute.
Files not reviewed (2)
  • packages/components/src/components/hds/form/label/index.hbs: Language not supported
  • showcase/app/templates/components/form/radio.hbs: Language not supported

KristinLBradley
KristinLBradley previously approved these changes May 9, 2025
@shleewhite
Copy link
Contributor

Nice! I just noticed that in the showcase, when the id is set to false it still has the ember generated id.

@didoo didoo merged commit 812fd64 into main May 9, 2025
16 checks passed
@didoo didoo deleted the fix-form-label-type branch May 9, 2025 18:19
@hashibot-hds hashibot-hds mentioned this pull request May 9, 2025
@alex-ju alex-ju added this to the [email protected] milestone May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants